Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iOS: use shaderc-rs for glsl to spirv compilation #324

Merged
merged 1 commit into from
Sep 1, 2020

Conversation

MichaelHills
Copy link
Contributor

@MichaelHills MichaelHills commented Aug 24, 2020

One of the steps to getting iOS working #87

I tested a few examples on mac sprite, sprite_sheet, and 3d_scene. On iOS I tested 3d_scene and a cut down version of spawner.

The motivating change for this was spawning glslValidator (or other processes) isn't going to work on iOS. Plus the bevy-glsl-to-spirv code said TODO use shaderc-rs.

@karroffel karroffel added A-Build-System Related to build systems or continuous integration A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on labels Aug 24, 2020
@cart
Copy link
Member

cart commented Aug 24, 2020

The biggest downsides here are that shaderc:

  1. has a big effect on compile times (unless you bundle precompiled binaries)
  2. is hard to build on windows (requires setting up the ninja build system and python). this makes onboarding difficult.
  3. precompiled binaries require setting environment variables, which complicates builds and makes "first time usage" hard.

Medium term, Naga will handle this and wont suffer from these problems.

Short term, I would only want to merge this if we do one of the following:

  1. Make shaderc-rs use precompiled binaries by default on all desktop platforms, with zero manual configuration required from users.
  2. Use shaderc on IOS and glsl-to-spirv everywhere else (using cargo features).

@MichaelHills
Copy link
Contributor Author

Makes sense, I prefer option 2 for now rather than making invasive changes to all platforms.

@MichaelHills
Copy link
Contributor Author

I've updated to use cfg switches. However I noticed that @lachlansneff seems to have started on some naga support? https://github.com/bevyengine/bevy/commits/a47749c794761cc4c13879a1638a77cecaa06804/crates/bevy_render/src/shader/preprocessor.rs

I thought I'd see if I could get naga working for iOS instead, and when I noticed that preprocessor support was lacking I stumbled across gfx-rs/naga#132

So maybe hold off on shaderc-rs, Naga would be better and doesn't have any of the iOS cmake issues since its pure rust.

@lachlansneff
Copy link
Contributor

Naga is quickly becoming mature enough to replace spirv-reflect in bevy.

@cart
Copy link
Member

cart commented Aug 25, 2020

I'm sold on waiting for Naga. That seems like the right call. But if you hit a roadblock / want to move forward now, im also willing to merge "shaderc but only for iOS"

@MichaelHills MichaelHills mentioned this pull request Aug 29, 2020
@MichaelHills MichaelHills changed the title Switch over to shaderc-rs for glsl to spirv compilation iOS: use shaderc-rs for glsl to spirv compilation Aug 29, 2020
@cart cart mentioned this pull request Aug 29, 2020
@naithar naithar mentioned this pull request Aug 30, 2020
@MichaelHills
Copy link
Contributor Author

Just rebased and re-pushed. I think we should just land this for now. We are getting some traction from multiple people on iOS support so let's land what we can to streamline the process.

There's still one outstanding issue with cmake-rs needing to be forked for now in order to compile shaderc-rs properly so this PR isn't enough on its own. It's just 1 less issue though. :) I'll keep an eye on Naga and try to port the iOS target to it when it's ready.

@cart
Copy link
Member

cart commented Sep 1, 2020

Sounds good to me. I also just realized we could make this a temporary solution to #130 if we can somehow make shaderc optional for non-ios platforms and required for ios.

But no sense in blocking on that. This is good to go.

@cart cart merged commit 38a982b into bevyengine:master Sep 1, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration A-Rendering Drawing game state to the screen C-Dependencies A change to the crates that Bevy depends on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants